Skip to content

Feature/visual room browser v4 - #8731

Open
odzhychko wants to merge 3 commits into
mainfrom
feature/visual-room-browser-v4
Open

Feature/visual room browser v4#8731
odzhychko wants to merge 3 commits into
mainfrom
feature/visual-room-browser-v4

Conversation

@odzhychko

@odzhychko odzhychko commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Cleaned up version of #8264
Resolves #8724

Before (main)

image

After

Screencast.From.2026-08-15.20-01-26.mp4

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.25000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/models/resourceProps.ts 44.44% 4 Missing and 1 partial ⚠️
src/utils/roomFilter.ts 99.15% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@odzhychko
odzhychko force-pushed the feature/visual-room-browser-v4 branch from 5cebb4a to 48a1ed8 Compare August 15, 2026 17:48
Co-authored-by:  Rikdekker <Rikdekker@users.noreply.github.com>
Assisted-by: ClaudeCode:claude-opus-5
Assisted-by: ClaudeCode:claude-opus-4.8
Signed-off-by: Oleksandr Dzhychko <hey@oleks.dev>
@odzhychko

Copy link
Copy Markdown
Contributor Author

@SebastianKrupinski Let me know if you find something with regards to code organization.
@nimishavijay Let me know if you find something with regards to UI/UX.

@SebastianKrupinski SebastianKrupinski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I tested this this works kind of:

Image

As mentioned already all the fancy spiting and join does not really work and only works for "Netherlands" style address format.

Comment thread src/utils/roomFilter.ts
Comment on lines +95 to +97
function isPostalCode(segment: string): boolean {
return /^\d{4,6}(\s*[A-Z]{1,2})?$/i.test(segment)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only matches:

4–6 digit numeric codes (covers most European codes like German 5-digit, or simple numeric formats)
Optionally followed by 1–2 letters, with optional whitespace before them (case-insensitive) — this is specifically the Dutch format (1098 XG)

It does not handle many common formats:

UK postal codes (SW1A 1AA) — letters before digits, doesn't match
Canadian codes (K1A 0B1) — letter-digit-letter pattern, doesn't match
US ZIP+4 (12345-6789) — hyphen not allowed
3-digit codes (some countries) — below the 4-digit minimum

@odzhychko odzhychko Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to usage of isPostalCode of deriveBuildingName:

Yeah, it's only a guess. That's why #8734 (incl. nextcloud/server#63244) is prossued.

@odzhychko odzhychko Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to usage of isPostalCode in joinAddressSegments for buildRoomLocation lets continue in #8731 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you fine with leaving until we can replace it with the building name from #8734?

Comment thread src/utils/roomFilter.ts Outdated
Comment on lines +127 to +145
function joinAddressSegments(segments: string[]): string {
const parts: string[] = []

for (let index = 0; index < segments.length; index++) {
const segment = segments[index]
const next = segments[index + 1]
// A postal code belongs with its city: "1098 XG, Amsterdam" reads as
// "1098 XG Amsterdam" on an envelope, and in a map application.
if (next !== undefined && isPostalCode(segment)) {
parts.push(`${segment} ${next}`)
index++
continue
}

parts.push(segment)
}

return parts.join(', ')
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are assuming that the address segments are always in the same order

Region Typical order vs. assumed order
Netherlands (assumed) Building, Street, Postal code, City ✅ matches
Germany/France Street, Postal code + City (one field) no building segment; postal code usually already joined with city
UK Street, Town, County, Postcode (postcode is last) postcode comes after the city, not before
US Street, City, State + ZIP ZIP is bundled with state, not directly adjacent to a bare "city" segment
Japan Postal code first, then Prefecture → City → Ward → Block → Building (largest → smallest, reversed) postal code is first, and the "building" is last

"150-0001, Tokyo, Shibuya, 1-1 Building"
the first segment "150-0001" isn't even matched as a postal code by isPostalCode (it has a hyphen, and starts with 3 digits), so it gets wrongly picked as the "building name" instead of the actual building at the end. the merge only fires when a postal-code-shaped segment is immediately followed by another segment,

"221B Baker Street, London, SW1A 2AA"
"SW1A 2AA" doesn't even match the regex, but even if it did, there's no next segment after it — so no merge happens,

@odzhychko odzhychko Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be removed with together with buildRoomLocation and tackled in a separate issue.
See #8731 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be removed with together with buildRoomLocation and tackled in a separate issue.

Done 4f1ac33

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate issue in #8735.

Comment thread src/utils/roomFilter.ts Outdated
* @param room Room to build a location for
* @return Location string, or null if the room carries no usable address data
*/
export function buildRoomLocation(room: RoomOption): string | null {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inherits all the the flaws already mentioned again.

Also breaks localization with hard coded concatenation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some background

  1. Why not just put roomBuildingAddress into LOCATION?
    • A "good" LOCATION should also contain the room name and maybe the room floor.

About current behaviour

  1. When adding a room through the resource search we use the roomAddress. (ResourceList.vue)
addResource({ commonName, email, calendarUserType, language, timezoneId, roomAddress }) {
	this.calendarObjectInstanceStore.addAttendee({
		...
	})
	this.updateLocation(roomAddress)
}
  1. roomAddress is derived in nextcloud/cdav-library (with regards to localization same bad ^^)
roomAddress: {
get: () => {
	const data = [
		this.roomBuildingRoomNumber,
		this.roomBuildingStory,
		this.roomBuildingAddress,
	]
	return data
		.filter((value) => !!value)
		.join(', ')
},

Going forward: Good solution

I see these possible good solution in the long term for this problem:

  1. Have the resource provider always provide a "good" roomAddress.
  2. Use a localization string like t('calendar', '{roomBuildingAddress}, {roomBuildingStory} (Room {roomBuildingRoomNumber}', ...)
  3. Use use roomAddress (as in 1.) if provided and fallback to (2.).

Going forward this PR

I will just use roomAddress to be consistant with what happens in ResourceList.vue so that we stay consistently bad 😆

Then we can tackle the problem of "Set a good LOCATION for selected room" in a separate issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will just use roomAddress to be consistant with what happens in ResourceList.vue so that we stay consistently

Done 4f1ac33

@odzhychko

Copy link
Copy Markdown
Contributor Author

Okay I tested this this works kind of:

Your screenshot looks of. Should look like in the description have you checked out feature/visual-room-browser-v4 (-v4)?

@SebastianKrupinski

Copy link
Copy Markdown
Contributor

Okay I tested this this works kind of:

Your screenshot looks of. Should look like in the description have you checked out feature/visual-room-browser-v4 (-v4)?

Yeah, I think so... but here is another take

image

@odzhychko

Copy link
Copy Markdown
Contributor Author

Okay I tested this this works kind of:

Your screenshot looks of. Should look like in the description have you checked out feature/visual-room-browser-v4 (-v4)?

Yeah, I think so... but here is another take

Looks still wrong...

image

Seating capacity should be indicate In the bottom left and not top right.

Signed-off-by: Oleksandr Dzhychko <hey@oleks.dev>
Signed-off-by: Oleksandr Dzhychko <hey@oleks.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏗️ In progress

Development

Successfully merging this pull request may close these issues.

[Enhancement] Allow Rooms and Resources dropdown to display all options on click/focus without requiring text input

2 participants